Skip to content

refactor(worker-javascript): extract Phase 1 helpers from Core.js#1036

Merged
dqnykamp merged 8 commits intoDoenet:mainfrom
dqnykamp:core-refactor-1
May 3, 2026
Merged

refactor(worker-javascript): extract Phase 1 helpers from Core.js#1036
dqnykamp merged 8 commits intoDoenet:mainfrom
dqnykamp:core-refactor-1

Conversation

@dqnykamp
Copy link
Copy Markdown
Member

@dqnykamp dqnykamp commented May 1, 2026

Summary

Phase 1 of breaking up packages/doenetml-worker-javascript/src/Core.js (13,837 lines, single class) into smaller composed TypeScript modules. No behavior change — every method/property previously on Core is preserved via thin delegating wrappers; all external callers (CoreWorker, tests, components, coreFunctions references) work unchanged.

Net effect: Core.js drops from 13,837 → 12,909 lines (-928, ~6.7%).

Modules extracted

Each follows the existing Dependencies.js / ParameterStack pattern: constructed with a core back-reference, accessed through this.core.<field> for shared hot state.

Module Lines Purpose
DiagnosticsManager.ts 182 Owns diagnostics[], hasPendingDiagnostics; source-location walk
StateVariableNameResolver.ts 275 Pure-function module: case-insensitive matching, alias substitution, array-entry detection
VisibilityTracker.ts 186 Owns visibilityInfo + save/suspend timers
StatePersistence.ts 149 Save-to-localStorage / database with throttle/debounce (separate from the essential-write engine, which stays in Core for now)
AutoSubmitManager.ts 60 Debounced answer-submit queue
NavigationHandler.ts 66 handleNavigatingToComponent, navigateToTarget
ResolverAdapter.ts 459 Adapter to the external (Rust) name resolver

Why now / why this shape

Core.js is the hardest file in the worker package to navigate. This refactor is purely a readability pass — no API change, no behavior change, no performance work. The full multi-phase plan extracts ~16-18 modules and ultimately drops Core.js to ~1,400 lines (constructor + generateDast orchestrator + thin delegators). This PR establishes the pattern with the safest, lowest-coupling extractions; later phases tackle the deep guts.

Addressed in review

A follow-up commit addresses all 9 inline review comments. Each item flagged code that pre-existed in Core.js and was either copied verbatim into the extracted modules or surfaced when the extraction converted JS to TS. None are refactor regressions.

  • Latent bug fix in ResolverAdapter.ts: createComponentIdx.primitive.number → guarded .value (matches the established primitive.type === "number" pattern in utils/resolver.ts and utils/componentIndices.ts).
  • Six fire-and-forget .catch() handlers added across StatePersistence (2), AutoSubmitManager (1), and VisibilityTracker (3) via a small shared reportTimerError helper, per AGENTS.md convention.
  • Four TS errors in ResolverAdapter.ts introduced by the JS→TS conversion: two FlatFragment literals were missing type: "flatFragment" (and one was missing parentSourceSequence); gatherDiagnosticsAndAssignDoenetMLRange's position? / sourceDoc? were spuriously optional and incompatible with assignDoenetMLRange's required arguments. All four resolved.
  • Typo fix: case-senstitivecase-sensitive in StateVariableNameResolver.ts.
  • Documentation consolidation: merged the unique sections from CLAUDE.md into AGENTS.md as the single source of truth (architecture, build commands, conventions, common tasks). CLAUDE.md is now a one-line pointer — Claude Code's auto-context still finds the project guide, and convention drift between the two files is eliminated.

Test plan

  • npm run build -w @doenet/doenetml-worker-javascript passes after every extraction
  • All 46 diagnostics tests pass (src/test/diagnostics)
  • All 91 copying tests pass (heavy alias/composite paths through StateVariableNameResolver and ResolverAdapter)
  • All 155 baseComponent tests pass
  • All 83 answerValidation tests pass (exercises AutoSubmitManager)
  • All 125 save/restore tests in callAction / mathlist / repeatForSequence pass (exercises StatePersistence)
  • Full Vitest suite for @doenet/doenetml-worker-javascript: 138 files, 2886 tests passed
  • npx tsc --noEmit clean after the review fixes
  • CI: full Vitest suite + Cypress groups 1–5

🤖 Generated with Claude Code

Begin breaking up the 13,837-line Core class by lifting seven self-
contained, low-coupling helpers into TypeScript modules. The pattern
matches the existing composed siblings (Dependencies.js, ParameterStack):
each module is constructed with a `core` back-reference, and Core retains
a thin delegating wrapper for every method/property that was previously
on the class so external callers (CoreWorker, tests, components, and
`coreFunctions`-bound references) continue to work unchanged.

Modules extracted:
- DiagnosticsManager.ts       — diagnostics queue + source-location walk
- StateVariableNameResolver.ts — pure-function name resolution utilities
- VisibilityTracker.ts        — visibility state and save/suspend timers
- StatePersistence.ts         — save to localStorage / database
- AutoSubmitManager.ts        — debounced answer-submit queue
- NavigationHandler.ts        — handleNavigatingToComponent, navigateToTarget
- ResolverAdapter.ts          — adapter to the external Rust name resolver

No behavior change. Core.js drops from 13,837 to 12,909 lines.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 1, 2026

⚠️ No Changeset found

Latest commit: 3998d92

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

dqnykamp and others added 2 commits May 1, 2026 14:15
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
dqnykamp added a commit to dqnykamp/DoenetML that referenced this pull request May 1, 2026
Adds .claude/settings.json with a PostToolUse hook that runs prettier on
files after Write/Edit/MultiEdit/NotebookEdit operations. Ensures code is
formatted automatically rather than relying on the agent to follow the
"format with prettier before committing" instruction in CLAUDE.md (which
was missed in PR Doenet#1036).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dqnykamp dqnykamp requested a review from Copilot May 1, 2026 19:41
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors packages/doenetml-worker-javascript/src/Core.js by extracting a first set of “Phase 1” helper managers/adapters into smaller TypeScript modules, keeping Core’s public API surface via delegating wrappers to improve navigability and maintainability of the worker core.

Changes:

  • Extracted diagnostics, visibility tracking, state persistence, navigation, auto-submit, and resolver adapter logic into new TS modules owned by Core via back-references.
  • Replaced large inlined blocks in Core.js with thin delegators to the extracted modules (preserving existing method/property names).
  • Added StateVariableNameResolver.ts as a pure-function module and wired Core wrappers to inject componentInfoObjects.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
packages/doenetml-worker-javascript/src/Core.js Wires new modules and replaces inlined logic with delegating wrappers to preserve Core’s API surface.
packages/doenetml-worker-javascript/src/DiagnosticsManager.ts New manager owning diagnostics array/pending flag and source-location lookup.
packages/doenetml-worker-javascript/src/VisibilityTracker.ts New module owning visibility timing aggregation and suspend/resume timers.
packages/doenetml-worker-javascript/src/StatePersistence.ts New module owning local/db persistence pipeline with debounce/throttle timers.
packages/doenetml-worker-javascript/src/AutoSubmitManager.ts New module for debounced answer auto-submit queue.
packages/doenetml-worker-javascript/src/NavigationHandler.ts New module for navigation-to-component / navigate-to-target host messaging.
packages/doenetml-worker-javascript/src/ResolverAdapter.ts New adapter module for translating component mutations into resolver flat fragments.
packages/doenetml-worker-javascript/src/StateVariableNameResolver.ts New pure utilities for state-variable name matching/public filtering/alias substitution/array-entry detection.
CLAUDE.md Adds repository guidance/documentation intended for Claude Code workflows.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/doenetml-worker-javascript/src/ResolverAdapter.ts Outdated
Comment thread packages/doenetml-worker-javascript/src/StatePersistence.ts Outdated
Comment thread packages/doenetml-worker-javascript/src/VisibilityTracker.ts Outdated
Comment thread packages/doenetml-worker-javascript/src/VisibilityTracker.ts Outdated
Comment thread packages/doenetml-worker-javascript/src/AutoSubmitManager.ts Outdated
Comment thread packages/doenetml-worker-javascript/src/StatePersistence.ts Outdated
Comment thread packages/doenetml-worker-javascript/src/VisibilityTracker.ts Outdated
Comment thread packages/doenetml-worker-javascript/src/StateVariableNameResolver.ts Outdated
Comment thread CLAUDE.md Outdated
Address 9 Copilot review comments on PR Doenet#1036. All flagged code (the
.primitive.number bug, six fire-and-forget Promises, the typo, four
TS errors) pre-existed in Core.js and was either copied verbatim or
exposed when the JS was converted to TS during extraction.

- ResolverAdapter: fix latent createComponentIdx.primitive.number access
  (should be .value, with a primitive.type === "number" guard) — same
  pattern as utils/resolver.ts:220-224 and utils/componentIndices.ts:725.
- ResolverAdapter: add type: "flatFragment" and parentSourceSequence
  fields to two FlatFragment literals; tighten gatherDiagnosticsAnd-
  AssignDoenetMLRange signature so position/sourceDoc match assign-
  DoenetMLRange's required arguments.
- Add reportTimerError helper and attach .catch() handlers to six
  fire-and-forget Promises in setTimeout callbacks across
  StatePersistence (2), AutoSubmitManager (1), and VisibilityTracker
  (3), per AGENTS.md convention. Also clear submitAnswersTimeout when
  it fires.
- Fix typo "case-senstitive" -> "case-sensitive" in
  StateVariableNameResolver.

Documentation: merge CLAUDE.md into AGENTS.md as the single source of
truth (project conventions, architecture, build commands, testing,
common tasks). Reduce CLAUDE.md to a one-line pointer so Claude Code
auto-context still finds the project guide while eliminating drift.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dqnykamp
Copy link
Copy Markdown
Member Author

dqnykamp commented May 3, 2026

Thanks for the review. All 9 comments are addressed in aed7910. Each one flagged code that pre-existed in Core.js and was either copied verbatim or surfaced when the JS was converted to TS during extraction.

Fixes by group:

  • Latent bug (ResolverAdapter.ts): createComponentIdx.primitive.number → guarded .primitive.value (matches utils/resolver.ts:220-224).
  • Six fire-and-forget Promises (StatePersistence ×2, AutoSubmitManager ×1, VisibilityTracker ×3): added .catch() handlers via a shared reportTimerError(label) helper in src/utils/timerErrors.ts. Also clear submitAnswersTimeout when the debounce fires.
  • Typo: case-senstitivecase-sensitive.
  • CLAUDE.md duplication: merged the unique sections from CLAUDE.md into AGENTS.md (architecture, build commands, conventions, common tasks) and reduced CLAUDE.md to a one-line pointer. AGENTS.md is now the single source of truth — Claude Code's auto-context still finds the project guide via the stub.

Bonus: while addressing the above, four pre-existing TS errors in ResolverAdapter.ts (introduced by the JS→TS conversion) were also fixed: two FlatFragment literals were missing the type: "flatFragment" discriminator (and one was missing parentSourceSequence); gatherDiagnosticsAndAssignDoenetMLRange's position? / sourceDoc? were spuriously optional and incompatible with assignDoenetMLRange. npx tsc --noEmit is now clean.

Full Vitest suite for @doenet/doenetml-worker-javascript passes (138 files / 2886 tests).

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/doenetml-worker-javascript/src/StatePersistence.ts Outdated
Comment thread AGENTS.md Outdated
Comment thread packages/doenetml-worker-javascript/src/StateVariableNameResolver.ts Outdated
dqnykamp and others added 2 commits May 2, 2026 23:39
Address the in-scope findings from the second-round review of the
Core.js helper extraction:

- DiagnosticsManager: replace local Diagnostic type with the existing
  DiagnosticRecord discriminated union from @doenet/utils; preliminary
  diagnostics now type-narrow via NonErrorDiagnosticRecord, addDiagnostic
  pushes the record directly, and the dedup check uses explicit branches.
- DiagnosticsManager: add markPending() and route the one external
  setter (Core.js error-component path) through it; drop the
  set hasPendingDiagnostics accessor on Core.
- Core.js: replace five aliased imports with a single
  import * as nameResolver namespace import.
- Core.js: remove six near-duplicate wrapper-block preambles; replace
  with one file-level explanation and short // → managerName markers.
- DiagnosticsManager: tighten getDiagnostics docstring (was misleading
  about the codebase accumulating messages); correct addDiagnostic
  return-value docstring (Core.js does inspect it for the rethrow path).
- StatePersistence: drop the "slated for Phase 4" reference.
- AGENTS.md: clarify Layer 2 Rust phrasing; expand the "Add a new
  component type" step about schema registration.

Defer the structural items (typing core: any, reducing stateless
managers to plain functions, moving getSourceLocationForComponent,
TimerLabels const, regression test for the .primitive.value fix)
to CORE_REFACTOR_DEFERRED.md for the next refactor pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address the three new Copilot comments from the second review pass:

- StatePersistence.saveImmediately: replace truthy check on
  saveDocStateTimeoutID with explicit `!== null`, matching scheduleSave
  at line 41, and null the field after clearTimeout for self-contained
  timer state (saveState happens to null it too, but the call site
  shouldn't depend on that).
- AGENTS.md: drop the redundant `-- run` from the
  test:all-no-worker-js command (the script already appends `-- run`
  internally), and tighten the description from "all packages except
  worker" to "all packages except `doenetml-worker-javascript`" — only
  that one worker package is excluded; doenetml-worker and
  doenetml-worker-rust are still included.
- StateVariableNameResolver.checkIfArrayEntry: typo
  "begins when an arrayEntry" -> "begins with an array entry".

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dqnykamp
Copy link
Copy Markdown
Member Author

dqnykamp commented May 3, 2026

Thanks for the round-2 follow-up. All 3 comments addressed in 8951df5.

  • StatePersistence.saveImmediately — replaced the truthy check on saveDocStateTimeoutID with explicit !== null (matches scheduleSave at line 41) and nulled the field after clearTimeout so the call site is self-contained instead of depending on saveState clearing it on entry.
  • AGENTS.md test command — dropped the redundant -- run (the test:all-no-worker-js script already appends -- run internally, so passing it again would double the arg) and tightened the description from "all packages except worker" to "all packages except doenetml-worker-javascript" (only that one worker package is in the exclude list; doenetml-worker and doenetml-worker-rust are still included).
  • Typocase-senstitive-style typo at StateVariableNameResolver.ts:250: "begins when an arrayEntry" → "begins with an array entry".

npx tsc --noEmit clean and callAction.test.ts (20/20 — exercises the StatePersistence path through saveImmediately) passing.

… and fix stale reference

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Adds a deferred-items entry listing three intentionally unawaited Promise
calls (Core.js:410, 11160, and 449-452) that pre-date Phase 1 and violate
the AGENTS.md fire-and-forget convention, with a suggested fix using the
reportTimerError helper added in this PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dqnykamp dqnykamp merged commit 915d2e0 into Doenet:main May 3, 2026
4 checks passed
@dqnykamp dqnykamp deleted the core-refactor-1 branch May 3, 2026 05:34
dqnykamp added a commit to dqnykamp/DoenetML that referenced this pull request May 3, 2026
Brings in the squash-merged Phase 1 (Doenet#1036) and Phase 2 (Doenet#1038) PRs along
with their post-review changes, and adopts the same review-driven patterns
in the remaining Phase 3 modules.

Phase 1/2 manager files (ActionTriggerScheduler, AutoSubmitManager,
ChildMatcher, ComponentLifecycle, DeletionEngine, DiagnosticsManager,
ProcessQueue, RendererInstructionBuilder, ResolverAdapter, StatePersistence,
StateVariableNameResolver, VisibilityTracker) plus AGENTS.md, CLAUDE.md,
and the new utils/timerErrors.ts and CORE_REFACTOR_DEFERRED.md are taken
from upstream.

Core.js merges Phase 3's extraction wrappers with upstream's Phase 1/2
review changes:
- import * as nameResolver (replaces aliased named imports)
- short-form `// → managerName` markers replace the long per-section
  comment blocks
- top-of-class block comment now lists all phases
- removed `set hasPendingDiagnostics` (callers go through markPending())
- replaced open-coded processQueueManager three-field reset with
  processQueueManager.reset()
- added `// → componentBuilder`, `// → compositeExpander`,
  `// → stateVariableDefinitionFactory`, `// → stateVariableInitializer`,
  `// → resolverAdapter` markers for the new sections

Phase 3 modules adopt the Phase 2 review conventions:
- ComponentBuilder.ts: `core.hasPendingDiagnostics = true` →
  `core.diagnosticsManager.markPending()` (the setter on Core was removed
  in the Phase 1 review)
- ComponentBuilder.ts and CompositeExpander.ts: standardize on
  `core._components` (was using the `core.components` getter for the same
  underlying array)

CORE_REFACTOR_DEFERRED.md updates:
- Type-the-`core: any` and stateless-managers items now list Phase 3
  managers alongside Phase 1/2.
- Pre-existing fire-and-forget Core.js line numbers updated for the
  post-Phase-3 layout (444, 4387, 483-486).
- Carried-over TODO/XXX inventory now includes the Phase 3 markers in
  StateVariableInitializer, StateVariableDefinitionFactory,
  ComponentBuilder, and CompositeExpander.
- _components/components convention note now covers Phase 3.

Verification:
- `tsc --noEmit` produces strictly fewer errors than the pre-merge backup
  (359 vs 365 — the removed setter and the `core._components` switch
  resolve a handful of `any` complaints; nothing new was introduced).
- callAction.test.ts (20/20 — exercises StatePersistence + ProcessQueue +
  ActionTriggerScheduler), circle.test.ts and spreadsheet.test.ts
  (65 passed, 1 todo — exercises StateVariableInitializer's
  array-callback path that the Phase 3 follow-up commit fixed) all green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants